Add phase-configs support to Hook model for target-app resolution#1812
Add phase-configs support to Hook model for target-app resolution#1812stiv03 wants to merge 9 commits intocloudfoundry:masterfrom
Conversation
stiv03
force-pushed
the
feature/hook-phase-target-app
branch
from
20deee7 to
0827e89
Compare
…reen deployments Route hook task execution to idle or live app based on phase-configs in the hook descriptor. Pass idleMtaColor, liveMtaColor, subprocessPhase and phase variables to all hook call activities in BPMN processes. Add determineDeploymentTypeSafely to avoid SERVICE_ID lookup failures in hook subprocesses. # Conflicts: # pom.xml
buildCurrentPhaseString was always picking the first phase from hook.getPhases(), causing wrong target-app resolution when a hook registered for multiple phases fired during a later phase. Also fixed getDeploymentTypeString returning "deploy" instead of "blue-green" inside hook subprocesses where SERVICE_ID is absent. Introduced HOOK_EXECUTION_PHASE variable set by HooksExecutor to record which phase triggered the current hook execution. ExecuteTaskStep now matches against this value to find the correct phase-config entry. Variable is passed to all hook call activities in deploy-app, undeploy-app, backup-existing-app and stop-dependent-modules BPMNs.
stiv03
force-pushed
the
feature/hook-phase-target-app
branch
from
0827e89 to
c5bab3b
Compare
stiv03
force-pushed
the
feature/hook-phase-target-app
branch
from
c5bab3b to
9165d29
Compare
| .toList()); | ||
| context.setVariable(Variables.DEPLOYMENT_DESCRIPTOR, descriptor); | ||
|
|
||
| validatePhasesConfig(context, descriptor); |
There was a problem hiding this comment.
extract this validation in a new class
| private void validateHookHasNoDuplicatePhaseConfigs(Hook hook) { | ||
| Object phasesConfigValue = hook.getParameters().get(SupportedParameters.PHASES_CONFIG); | ||
| if (!(phasesConfigValue instanceof List)) { | ||
| return; |
There was a problem hiding this comment.
shouldn't the deployment fail if phases-config is not a List?
|
|
||
| @SuppressWarnings("unchecked") | ||
| private void validateHookHasNoDuplicatePhaseConfigs(Hook hook) { | ||
| Object phasesConfigValue = hook.getParameters().get(SupportedParameters.PHASES_CONFIG); |
There was a problem hiding this comment.
Null handling?
| @Override | ||
| protected StepPhase executeStep(ProcessContext context) { | ||
| CloudApplicationExtended app = context.getVariable(Variables.APP_TO_PROCESS); | ||
| Hook hook = context.getVariable(Variables.HOOK_FOR_EXECUTION); |
There was a problem hiding this comment.
Instead of passing it as argument, just get it in resolveTargetAppName
| .orElseGet(() -> hook.getPhases() | ||
| .stream() | ||
| .filter(p -> p.contains(APPLICATION_PHASE_SUBSTRING)) | ||
| .findFirst() | ||
| .orElse("")); |
There was a problem hiding this comment.
Why are we searching for the first application hook?
| String idleSuffix = BlueGreenApplicationNameSuffix.IDLE.asSuffix(); | ||
|
|
||
| if (HookPhaseProcessType.HookProcessPhase.IDLE.getType().equals(targetApp)) { | ||
| if (appName.endsWith(liveSuffix)) { |
There was a problem hiding this comment.
When blue-green.application.before-start.idle is used. The apps are still called (for example):
cf-app-with-hooks-idle
and:
cf-app-with-hooks-live
If we remove the suffix we will get a name of application that does not exist.
| return appName + idleSuffix; | ||
| } | ||
| } | ||
| if (HookPhaseProcessType.HookProcessPhase.LIVE.getType().equals(targetApp)) { |
There was a problem hiding this comment.
Can we simplify these conditions as they are hard to read?
We have only one hook that is executed before Phase.AFTER_RESUME of the blue-green (blue-green.application.before-start.idle).
When this hook is used apps are called:
cf-app-with-hooks-idle
cf-app-with-hooks-live
So we can just remove the suffix from appName and append idle or live. (more readable) BlueGreenApplicationNameSuffix::removeSuffix
--
All other hooks are executed after Phase.AFTER_RESUME. Then apps are called:
cf-app-with-hooks
cf-app-with-hooks-live
So if idle needed -> remove suffix, if live needed remove suffix append Live. (no conditions at all)
| private String swapColorSuffix(String appName, ApplicationColor fromColor, ApplicationColor toColor) { | ||
| String fromSuffix = fromColor.asSuffix(); | ||
| String toSuffix = toColor.asSuffix(); | ||
| if (appName.endsWith(fromSuffix)) { | ||
| return appName.substring(0, appName.length() - fromSuffix.length()) + toSuffix; | ||
| } | ||
| if (!appName.endsWith(toSuffix)) { | ||
| return appName + toSuffix; | ||
| } |
There was a problem hiding this comment.
Can you just remove suffix BlueGreenApplicationNameSuffix::removeSuffix then append toColor?
| public ProcessType determineDeploymentTypeSafely(ProcessContext context) { | ||
| return processTypeParser.getProcessType(context.getExecution(), false); | ||
| } |
There was a problem hiding this comment.
What is this?
| context.setVariable(Variables.HOOK_EXECUTION_PHASE, hooksWithPhases.getHookPhases() | ||
| .stream() | ||
| .findFirst() | ||
| .map(HookPhase::getValue) | ||
| .orElse(null)); |
There was a problem hiding this comment.
This variable breaks the idea of having multiple phases for one step. While technically:
getHookPhasesBeforeStep
getHookPhasesAfterStep
always return only one hook phase, in future they can be many, so this variable should comply with one to many relationship (step to hook phases)
2 participants
No description provided.